Ensure consistent drop for panicking drop in hint::select_unpredictable #145174
+71
−2
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There are a few alternatives to the implementation. The principal problem is that the selected value must be owned (in the sense of having any drop flag of sorts) when the unselected value is dropped, such that panic unwind goes through the drop of both. This ownership must then be passed on in return when the drop went smoothly. The basic way of achieving this is by extracting the selected value first, at the cost of relying on the optimizer a little more for detecting the copy as constructing the return value despite having a place in the body.
rustbot
added
S-waiting-on-review
197g
changed the title
This comment has been minimized.
This comment has been minimized.
It seems important for LLVM that we select on the values by-value instead of reading and have no intermediate store. So make sure the guards selects both potential drops but defers the return value to the second selection. Since the two options alias we use raw mutable pointers instead of mutable references as before.
joboet
requested changes
Aug 30, 2025
joboet
reviewed
Aug 30, 2025
Instead of a tuple, select the dropped value and its guard with two separate calls to the intrinsic which makes both calls have a pointer-valued argument that should be simpler in codegen. Use the same condition on all (not an inverted condition) to clarify the intent of parallel selection. This should also be a simpler value-dependency chain if the guard is deduced unused (i.e. drop_in_place a noop for the type).
197g
force-pushed
the
issue-145148-select-unpredictable-drop
branch
from
f1c2b28 to
ee9803a
Compare
|
@bors r+ |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Aug 31, 2025
Rollup of 4 pull requests Successful merges: - #144443 (Make target pointer width in target json an integer) - #145174 (Ensure consistent drop for panicking drop in hint::select_unpredictable) - #145592 (Fix format string grammar in docs and improve alignment error message for #144023) - #145931 (Clarify that align_offset overaligns) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
that referenced
this pull request
Aug 31, 2025
Rollup merge of #145174 - 197g:issue-145148-select-unpredictable-drop, r=joboet Ensure consistent drop for panicking drop in hint::select_unpredictable There are a few alternatives to the implementation. The principal problem is that the selected value must be owned (in the sense of having a drop flag of sorts) when the unselected value is dropped, such that panic unwind goes through the drop of both. This ownership must then be passed on in return when the drop went smoothly. The basic way of achieving this is by extracting the selected value first, at the cost of relying on the optimizer a little more for detecting the copy as constructing the return value despite having a place in the body. Unfortunately, that causes LLVM to discard the !unpredictable annotation (for some reason that is beyond my comprehension of LLVM). <details> <summary>Extract from the build log showing an unannotated select being used</summary> ``` 2025-08-09T16:51:06.8790764Z 39: define noundef i64 `@test_int2(i1` noundef zeroext %p, i64 noundef %a, i64 noundef %b) unnamed_addr #0 personality ptr `@rust_eh_personality` { 2025-08-09T16:51:06.8791368Z check:47'0 X~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found 2025-08-09T16:51:06.8791700Z 40: start: 2025-08-09T16:51:06.8791858Z check:47'0 ~~~~~~~ 2025-08-09T16:51:06.8792043Z 41: %ret.i = select i1 %p, i64 %a, i64 %b 2025-08-09T16:51:06.8792293Z check:47'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 2025-08-09T16:51:06.8792686Z check:47'1 ? possible intended match 2025-08-09T16:51:06.8792946Z 42: ret i64 %ret.i 2025-08-09T16:51:06.8793127Z check:47'0 ~~~~~~~~~~~~~~~~ ``` </details> So instead, this PR includes a guard to drop the selected `MaybeUnit<T>` which is active only for the section where the unselected value is dropped. That leaves the code for selecting the result intact leading to the expected ir. That complicates the 'unselection' process a little bit since we require _both_ values as a result of that intrinsic call. Since the arguments alias, this portion as well as the drop guard uses raw pointers. Closes: #145148 Prior: #139977
github-actions bot
pushed a commit
to rust-lang/compiler-builtins
that referenced
this pull request
Sep 1, 2025
Rollup of 4 pull requests Successful merges: - rust-lang/rust#144443 (Make target pointer width in target json an integer) - rust-lang/rust#145174 (Ensure consistent drop for panicking drop in hint::select_unpredictable) - rust-lang/rust#145592 (Fix format string grammar in docs and improve alignment error message for rust-lang/rust#144023) - rust-lang/rust#145931 (Clarify that align_offset overaligns) r? `@ghost` `@rustbot` modify labels: rollup
github-actions bot
pushed a commit
to rust-lang/miri
that referenced
this pull request
Sep 1, 2025
Rollup of 4 pull requests Successful merges: - rust-lang/rust#144443 (Make target pointer width in target json an integer) - rust-lang/rust#145174 (Ensure consistent drop for panicking drop in hint::select_unpredictable) - rust-lang/rust#145592 (Fix format string grammar in docs and improve alignment error message for rust-lang/rust#144023) - rust-lang/rust#145931 (Clarify that align_offset overaligns) r? `@ghost` `@rustbot` modify labels: rollup
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There are a few alternatives to the implementation. The principal problem is that the selected value must be owned (in the sense of having a drop flag of sorts) when the unselected value is dropped, such that panic unwind goes through the drop of both. This ownership must then be passed on in return when the drop went smoothly.
The basic way of achieving this is by extracting the selected value first, at the cost of relying on the optimizer a little more for detecting the copy as constructing the return value despite having a place in the body. Unfortunately, that causes LLVM to discard the !unpredictable annotation (for some reason that is beyond my comprehension of LLVM).
Extract from the build log showing an unannotated select being used
So instead, this PR includes a guard to drop the selected
MaybeUnit<T>which is active only for the section where the unselected value is dropped. That leaves the code for selecting the result intact leading to the expected ir. That complicates the 'unselection' process a little bit since we require both values as a result of that intrinsic call. Since the arguments alias, this portion as well as the drop guard uses raw pointers.Closes: #145148
Prior: #139977